GH-142002: Revert GC changes to "work_to_do" logic.#142001
GH-142002: Revert GC changes to "work_to_do" logic.#142001nascheme wants to merge 1 commit intopython:mainfrom
Conversation
This reverts parts of the pythonGH-140262 change. The changes that affect the tuple untracking are left unchanged. Revert the changes to the calculation of the increment size, based on the "work_to_do" variable. This causes cyclic garbage to be collected more quickly. Revert also the change to test_gc.py, which was done because the expected GC collection was taking longer to happen. With the tuple untrack change, the performance regression as reported by bug pythonGH-139951 is still resolved (work_to_do changes are not required).
|
@sergey-miryanov given your recent good work on GC related things, you might want to take a look at this. This PR is an attempt at a quick fix, making the code work like the 3.14.0 release in this respect. However, for 3.15 I'm thinking we need to have a more careful look at how the GC increment size is computed. I'm going to create another PR that adds a "mark-alive" pass to the full GC run. That gives about a 2X performance improvement for full (manual) GC runs. |
|
🤖 New build scheduled with the buildbot fleet by @nascheme for commit fef3eed 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F142001%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
@nascheme Thanks! I will take a look.
|
|
If this lands, it'll need a backport because #140447 landed in 3.14. |
|
gc_report_cycles.py produces tiny cycles that would never survive the first generation in the old GC. That doesn't seem very realistic. We already have https://github.com/python/cpython/blob/main/Lib/test/_test_gc_fast_cycles.py to test that the amount of garbage stays within reasonable bounds, but it does produce larger, longer lived cycles. |
|
@sergey-miryanov
It might be worth revisiting the calculation.
|
I don't think whether they are tiny or not is very relevant. If this cyclic garbage is holding on to a lot of memory, the increase in memory usage due to the GH-140262 change would be much worse. IMO, we should be careful about changing how quickly the cyclic GC frees cyclic garbage in a bugfix release. At least, there should be some discussion and justification on why changing it is okay. GH-140262 doesn't even mention this change, let alone justify it. It changes the unit tests to make the expected iterations before collection 10x higher. Is that okay? I don't know.
Could be removed. It was a quick and dirty test adapted from a different test. A more realistic test (perhaps) would be to continuously create new objects, with a fraction of them being cyclic garbage. Then we could track peak memory usage and get a better idea of how these
The fact that we had to change that test to run in a subprocess, in order to make it more reliable, is not a good sign. We don't have unit tests that simulate a program that allocates a large number of objects (e.g. a couple million, at least) and creates some significant fraction of garbage cycles (say 10% of allocations). |
More tests are always good. Maybe we could parameterize _test_gc_fast_cycles.py to do that.
I'd love to base our heuristics on memory usage, rather than object count, but we really don't have enough control over the allocator to do that. Maybe in the future.
It was moved to make it more predictable. It means we can use lower thresholds giving a stricter test, as we don't need to account for the worse case heap left over by previous tests. |
|
Working on #142516 I got following: I took @nascheme script that creates SSL context and stores it in the instances with cycles and did some measurements. For 3.14.0, the script uses less memory - it grows up in few steps and stabilizes at 104.3265. GC runs after the following iterations: 700, 1600, 2600, 3600, 4600, 5600, 6600 and 7600. Each time the garbage collector runs, it successfully clears cycles and SSL contexts. I suspect the memory grows because we don't return it all at once to the OS. However, since we clear all previously allocated SSL contexts, subsequent calls reuse that memory. For 3.14.1+, the script uses about four times more memory, which grows and then stabilizes after the first gc.collect() at 488.934. GC runs after the following iterations: 700, 1700, 2700, 3700, 4600, 5600, 6600 and 7600. However, it only performs real work after 4600 iteration. This is because Full results here - https://github.com/sergey-miryanov/ssl-context-size I believe we should apply this patch until we get another schema for incremental GC. |
|
There is a reproducer for #142516 without ssl module: On main it grows to |
I love this producer. I went ahead and ran it for my problem case as well:
#!/usr/bin/env python
import gc
import socket
from prometheus_client import CollectorRegistry, Gauge, pushadd_to_gateway
REGISTRY = CollectorRegistry(auto_describe=True)
HOST = socket.gethostname()
LAST_RUN = Gauge("last_run_time", "Last time job has run.", registry=REGISTRY)
def push_metrics(job, registry):
pushadd_to_gateway("http://pushgateway:9091", job=job, registry=registry, grouping_key={"instance": HOST})
def get_rss():
import psutil
p = psutil.Process()
mem_info = p.memory_info()
rss = mem_info.rss
return rss / 1024.0
def main():
gc.collect(2)
rss = get_rss()
N = 5_000
for i in range(N):
push_metrics("background_tasks", REGISTRY)
if i % 100 == 0:
rss2 = get_rss()
size = rss2 - rss
print(f"idx {i:>5} size {size:>10_} kB")
if __name__ == "__main__":
main()With 25+ jobs running on a 16 GB staging machine that is reset at least every week, I've never ran out of RAM up until 3.14(.3) and here you can see why. Note that every time pushadd_to_gateway is called without handler arg, the prometheus_client library creates a new handler function. python-3.14.3 from the snippet: python-3.13.12 from the snippet: My current workaround is to call python-3.14.3 after adding |
This reverts parts of the GH-140262 change. The changes that affect the tuple untracking are left unchanged. Revert the changes to the calculation of the increment size, based on the "work_to_do" variable. This causes cyclic garbage to be collected more quickly. Revert also the change to test_gc.py, which was done because the expected GC collection was taking longer to happen.
With the tuple untrack change, the performance regression as reported by bug GH-139951 is still resolved (work_to_do changes are not required).
One way to test the effect of this change is to run the following quick-and-dirty script. It creates many reference cycles and then counts how many are not yet collected by the GC. At the end of the run, the maximum number of outstanding garbage objects are printed. With Python 3.13 (non-incremental GC), I get a max of less than 100 objects. With 3.14.0, I get approximately 2000 as the max. With the "main" branch (and 3.14 branch), I get 10,000 as the max.
gc_report_cycles.py